Skip to content

fix: Changing screen layout might make dock exclusion zone incorrect #958

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
deepin-bot merged 1 commit into from
Dec 25, 2024
Merged

fix: Changing screen layout might make dock exclusion zone incorrect #958

deepin-bot merged 1 commit into from
Dec 25, 2024

Conversation

@ArchieMeng
Copy link
Contributor

@ArchieMeng ArchieMeng commented Dec 11, 2024

Changing screen layout may not change the geometry of the screen binded to dock. (For example, the screen stay on top or left without resolution changed) So, connect slot to all screens instead.

pms: BUG-292677
Log: Changing screen layout might make dock exclusion zone incorrect

@deepin-ci-robot
Copy link

deepin-ci-robot commented Dec 12, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ArchieMeng, tsic404

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@deepin-bot
Copy link

deepin-bot bot commented Dec 20, 2024

TAG Bot

New tag: 1.0.10
DISTRIBUTION: unstable
Suggest: synchronizing this PR through rebase #976

@ArchieMeng
fix: Changing screen layout might make dock exclusion zone incorrect
33c7988 
Changing screen layout may not change the geometry of the screen binded
to dock. (For example, the screen stay on top or left without resolution
changed) So, connect slot to all screens instead.

pms: BUG-292677
Log: Changing screen layout might make dock exclusion zone incorrect
@deepin-ci-robot
Copy link

deepin-ci-robot commented Dec 24, 2024

deepin pr auto review

代码审查意见：

  1. 代码重复：在connect语句中，m_positionChangedTimer和m_exclusionZoneChangedTimer的连接逻辑几乎完全相同，可以考虑将这部分逻辑抽象成一个函数，减少代码重复。

  2. 使用static_cast：在连接信号和槽时使用static_cast是正确的，但可以添加注释说明为什么需要这样做，以便其他开发者理解。

  3. 信号连接的顺序：在screenChanged槽函数中，先启动m_positionChangedTimer，然后是m_exclusionZoneChangedTimer。这种顺序可能会导致onPositionChanged先于onExclusionZoneChanged执行，如果这两个槽函数之间存在依赖关系，可能需要调整顺序。

  4. 内存管理：如果m_dlayerShellWindow和m_window在LayerShellEmulation对象的生命周期内始终有效，那么不需要在screenChanged槽函数中重新连接信号。如果这些对象可能会被销毁，那么在重新连接信号之前应该确保旧的连接已经被断开。

  5. 性能考虑：使用定时器来处理频繁的信号变化是一种常见的做法，但需要确保定时器的间隔设置合理，以避免不必要的性能开销。这里设置的100毫秒间隔是一个合理的值吗？需要根据实际情况进行调整。

  6. 代码可读性：在screenChanged槽函数中，使用m_positionChangedTimer.start()和m_exclusionZoneChangedTimer.start()来启动定时器，这种方式比使用QMetaObject::invokeMethod更直接，提高了代码的可读性。

  7. 资源管理：在LayerShellEmulation的析构函数中，应该确保所有连接的信号和槽都被断开，以避免潜在的内存泄漏。

  8. 注释和文档：虽然代码中已经有了一些注释，但可以进一步添加更多的注释，特别是在复杂的逻辑和关键步骤上，以提高代码的可维护性。

综上所述，代码在逻辑上没有明显的问题，但在代码质量和性能优化方面还有改进的空间。

@ArchieMeng
Copy link
Contributor Author

ArchieMeng commented Dec 24, 2024

/recheck

@ArchieMeng
Copy link
Contributor Author

ArchieMeng commented Dec 25, 2024

/forcemerge

@deepin-bot
Copy link

deepin-bot bot commented Dec 25, 2024

This pr force merged! (status: blocked)

@deepin-bot deepin-bot bot merged commit 7d6a28b into linuxdeepin:release/beige Dec 25, 2024
7 of 10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tsic404 tsic404 tsic404 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@ArchieMeng @deepin-ci-robot @tsic404